-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POC][IR] Initial stab at std::string->String upgrade #5438
Conversation
cc @jroesch @icemelon9 |
def _update_from_std_str(key): | ||
def _convert(item, nodes): | ||
str_val = item["attrs"][key] | ||
jdata = json.loads(tvm.ir.save_json(tvm.runtime.String(str_val))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we bring the serialization of tvm:String out of save_json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate a bit about what do you mean? Right now all tvm objects are serialized through save_json, what we are doing here is to get that particular entry and append to the end of the nodes.
@jroesch @zhiics {type_key: "runtime.String", repr_str: "mystring"} If the content of the string is not printable, then it will show up as {type_key: "runtime.String", repr_b64: "base64encoding"} So the new IR entry will look like (note that the name field actually stores the index to the node table
This mechanism is baked into json serialization so it is a bit hard to move out. Of course we could also say that String should mostly be printable, and use a special serialization(e.g. just print as str when it is an old str attribute) Thet is a special case though and makes the json serialization harder to work with when directly serialize a String object itself. |
@tqchen ahh, I see. I was thinking if it is necessary to have some stuff like Load/Save for tvm::String separately, so that we don't need to put them under serialization.cc. Looks it is not really needed. Thanks. |
This PR is an example of String Update with backward compact(see json_compact.py)